Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated proto pb_plugin #347

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Updated proto pb_plugin #347

wants to merge 7 commits into from

Conversation

ShafSpecs
Copy link

Whilst attempting to fix issues within MAVSDK-Python outlined here, I noticed dependency conflicts arising from the fact that the pb_plugin utilises very old versions of protobuf.

Updated those dependencies and formatted the project. Also fixed a few failing tests

@ShafSpecs
Copy link
Author

Not sure if this is just my IDE going haywire or is a legit issue, but google.protobuf.compiler.plugin_pb2 seem to lack any exports

request = plugin_pb2.CodeGeneratorRequest()

Even though the protobuf plugin header file seems to contain it

@ShafSpecs
Copy link
Author

Not sure if this is just my IDE going haywire or is a legit issue, but google.protobuf.compiler.plugin_pb2 seem to lack any exports

request = plugin_pb2.CodeGeneratorRequest()

Even though the protobuf plugin header file seems to contain it

It looks like a breaking change from protobuf. CodeGeneratorResponse is no longer exported from compiler

JonasVautherin
JonasVautherin previously approved these changes Jun 18, 2024
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried it but it looks good to me 😇

@ShafSpecs
Copy link
Author

ShafSpecs commented Jun 19, 2024

Added a new commit to fix circular imports bugs + removed relative import. Renamed a few files as well (enum.py -> pb_enum.py as well as struct.py). Internal imports were resolving to the local files for some reason 👍

Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about this all but from far away it passes the squint test :).

I would say we merge this and the dependent PRs towards MAVSDK v3.

@ShafSpecs
Copy link
Author

👍. Agreed, btw do we have a milestone or project layout for Mavsdk v3 and it's focus? Meaning the major things that are to be accomplished? I've been using rust mavlink for a while now and have a few ideas that could help improve Mavsdk overall

@julianoes
Copy link
Collaborator

@ShafSpecs mavlink/MAVSDK#2316

@ShafSpecs
Copy link
Author

Nice 🚀!

  • Going through the code, my biggest pain point and something I would like to see in v3 is non-blocking asynchronous subscription. Not sure if this a server or MAVSDK-Python issue, but the hanging issue I raised here (with status text monitor) where the generator hangs till a message is sent instead of having a background pub/sub architecture. With mavlink, it's first come, first serve, some messages get sent on a regular basis (heartbeat) whilst others randomly (statustext).
  • And the server rpc stream not being synchronous in terms of how messages are queued, when a lot of plugins are subscribed to, errors about non-complete messages start to appear.
  • I see someone has also raised the udp/tcp issue too for uniformity 👍

My cpp isn't too sharp, so I am still studying the codebase (albeit slowly) but I have a hunch that the communication process can be improved to allow for more seamless data transfer. Thx

@JonasVautherin
Copy link
Collaborator

Not sure if this a server or MAVSDK-Python issue, but the hanging issue I raised mavlink/MAVSDK-Python#698 (comment) (with status text monitor) where the generator hangs till a message is sent

Hmm it's an async generator in asyncio, it is supposed to "hang" until it receives an event. Not sure if I understand your point correctly. What else would you have an async generator do? Until it returns, it has to "hang" until it receives the next event 🤔.

If you need to run multiple async generators in parallel, you can put them in their own coroutine, like e.g. in the mission example.

I don't know why there was a StatusCode.UNAVAILABLE in this particular issue, though. In the code you share there, you seem to create an async generator for each event, which under the hood creates a new connection every time (instead of leveraging the gRPC streams, I would say). So maybe after some amount of connections it just stops answering (maybe some kind of backpressure, or maybe the connections are not getting closed correctly under the hood?). But in any case it feels suboptimal to use an async generator like this 🤔.

@ShafSpecs
Copy link
Author

In the code you share there, you seem to create an async generator for each event, which under the hood creates a new connection every time (instead of leveraging the gRPC streams, I would say).

How would I leverage gRPC streams in that kind of situation (without resorting to new generators for every event?

Looking at the v3 milestone, most of my suggestions are already penned down, that would be all from me ❤️

@JonasVautherin
Copy link
Collaborator

How would I leverage gRPC streams in that kind of situation (without resorting to new generators for every event?

I haven't seen your code base, but my guess is that you want to poll for the drone state (instead of receiving events in the async generator).

What I would do in that case is some kind of Collector that would subscribe to all the streams I want and that would save the latest state. At the same time it would expose an interface allowing to get the latest state.

Some quick pseudo-code:

class Collector {
    health = None
    position = None

    async def run_observers():
        asyncio.ensure_future(observe_health())
        asyncio.ensure_future(observe_position())

    async def observe_health():
        async for new_health in drone.telemetry.health():
            health = new_health

    async def observe_position():
        async for new_position in drone.telemetry.position():
            position = new_position
}

From the outside you run Collector.run_observers() in a coroutine, which will continuously update the state of Collector. And when you want to access the latest position, you read Collector.position.

Would that help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants